Introduce Policy custom resource#5152
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new namespaced Policy CRD (group fleet.cattle.io/v1alpha1) used to constrain GitRepo, HelmOp, and Bundle resources living in the same namespace. The new resource supports top-level requireServiceAccount / allowedServiceAccounts checks plus per-kind sub-specs that carry defaults and allow-lists (regex for repo/chart, plain list for secrets). A shared policyrestrictions package aggregates multiple Policies (OR for bools, union for lists, first-non-empty for defaults, sorted by name for determinism). The GitRepo and HelmOp reconcilers validate-and-default; the Bundle reconciler validates only (closing the direct fleet-apply bypass). CRD, RBAC, deepcopy, controller plumbing, unit and e2e tests are included.
Changes:
- New
PolicyCRD/types, deepcopy, controller interface, CRD manifest, and RBAC entries forpolicies. - New
policyrestrictionsaggregation helpers; wiring into GitRepo, HelmOp, and Bundle reconcilers, with the GitRepo path now persisting injected defaults viar.Update. - Unit tests for each reconciler integration plus a new
e2e/single-cluster/policy_test.goexercising rejection/injection scenarios.
Reviewed changes
Copilot reviewed 20 out of 23 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/apis/fleet.cattle.io/v1alpha1/policy_types.go | Defines Policy, GitRepoPolicySpec, HelmOpPolicySpec, PolicyList. |
| pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go | Generated deepcopy for new types. |
| pkg/generated/controllers/fleet.cattle.io/v1alpha1/{policy,interface}.go | Generated controller/client interfaces for Policy. |
| internal/cmd/controller/policyrestrictions/policyrestrictions.go | Shared aggregation + allow-list helpers. |
| internal/cmd/controller/gitops/reconciler/restrictions.go | Merges GitRepoRestriction and Policy enforcement for GitRepo. |
| internal/cmd/controller/gitops/reconciler/gitjob_controller.go | Persists spec defaults via r.Update after authorize step. |
| internal/cmd/controller/gitops/reconciler/{restrictions,gitjob}_test.go | Updates mock List handler; adds Policy regex test cases. |
| internal/cmd/controller/helmops/reconciler/restrictions.go | Policy enforcement + defaulting for HelmOp. |
| internal/cmd/controller/helmops/reconciler/helmop_controller.go | Calls authorize step and records event on violation. |
| internal/cmd/controller/helmops/reconciler/{restrictions_test.go, helmop_controller_test.go} | New unit tests / Policy List mock expectations. |
| internal/cmd/controller/reconciler/bundle_policy.go | Bundle-level (validate-only) policy enforcement. |
| internal/cmd/controller/reconciler/bundle_controller.go | Invokes authorizeBundle and records PolicyViolation events. |
| internal/cmd/controller/reconciler/{bundle_policy_test.go, bundle_controller_test.go} | Bundle policy unit tests + Policy List expectation in helper. |
| charts/fleet-crd/templates/crds.yaml | Adds the Policy CRD. |
| charts/fleet/templates/rbac_{gitjob,helmops}.yaml | Grants list/get/watch on policies. |
| e2e/single-cluster/policy_test.go, e2e/assets/policy/*.yaml | End-to-end coverage for GitRepo, HelmOp and Bundle paths. |
Files not reviewed (1)
- pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sort.Slice(policies, func(i, j int) bool { | ||
| return policies[i].Name < policies[j].Name | ||
| }) | ||
|
|
||
| var m Merged | ||
| for _, p := range policies { |
| // Policy restrictions: validate the Bundle before producing any BundleDeployments. | ||
| // This closes the direct fleet-apply bypass path: a Bundle that violates policy is | ||
| // never deployed regardless of how it was created. | ||
| if err := r.authorizeBundle(ctx, bundle); err != nil { | ||
| return ctrl.Result{}, r.updateErrorStatus(ctx, bundleOrig, bundle, err) | ||
| } |
There was a problem hiding this comment.
Adding a Policy watch that fans out to every resource in a namespace is unnecessary. GitRepo and HelmApp self-recover via backoff retry (plus
GitRepo's 2h resync). Standalone Bundles are operator-initiated and operator-unblocked — the TerminalError fits that model. The risk of
fan-out churn outweighs the latency gain for a rarely-changed policy object.
| // Persist any spec defaults injected by AuthorizeAndAssignDefaults (e.g. defaultServiceAccount). | ||
| // The spec update bumps the generation and triggers a fresh reconcile. | ||
| if !reflect.DeepEqual(oldSpec, &gitrepo.Spec) { | ||
| if err := r.Update(ctx, gitrepo); err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
| return ctrl.Result{}, nil | ||
| } |
There was a problem hiding this comment.
This was the intended behavior from the beginning and if those resources are managed externally, then they should provide defaults themselves or the configuration needs to change not to require defaults.
After upgrade, only those resources will be affected that have been previously been configured by GitRepoRestriction to contain a default. The drift between externally managed resources and actual states becomes obvious and must be addressed externally (or by a configuration change).
| }, | ||
| ) | ||
| // AuthorizeBundle lists Policies in the namespace; return empty list so no policy is enforced. | ||
| mockCli.EXPECT().List(gomock.Any(), gomock.AssignableToTypeOf(&fleetv1.PolicyList{}), gomock.Any()).Return(nil) |
| r.Recorder.Eventf( | ||
| helmop, | ||
| nil, | ||
| corev1.EventTypeWarning, | ||
| "FailedToApplyRestrictions", | ||
| "ApplyPolicyRestrictions", | ||
| "%v", | ||
| err, | ||
| ) | ||
| return ctrl.Result{}, updateErrorStatusHelm(ctx, r.Client, req.NamespacedName, helmop, err) |
- Clone policies slice in Aggregate before sorting to avoid mutating the caller's backing array - Align event reason to PolicyViolation across GitRepo, HelmOp, and Bundle reconcilers - Use AnyTimes() on Policy List mock expectation so tests that reconcile multiple passes compose correctly
Additional Information
Checklist